-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Prevent excessive fetch calls #54
Conversation
@@ -30,6 +30,7 @@ final unleash = UnleashClient( | |||
url: Uri.parse('https://<your-unleash-instance>/api/frontend'), | |||
clientKey: '<your-client-side-token>', | |||
appName: 'my-app'); | |||
unleash.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added missing start method in README
lib/unleash_context.dart
Outdated
@@ -29,4 +29,22 @@ class UnleashContext { | |||
|
|||
return params; | |||
} | |||
|
|||
Map<String, String> toFlatMap() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nested properties from toMap are sent to the backend. toFlatMap is used to create a structure that matches setContextField format that is always flat e.g. setContextField('myField', 'myValue')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added propert eq and hashCode instead
await unleash.updateContext(UnleashContext( | ||
userId: '123', | ||
remoteAddress: 'address', | ||
sessionId: 'session', | ||
properties: {'customKey': 'customValue'})); | ||
// update whole context before start | ||
await unleash.updateContext(UnleashContext( | ||
userId: '123', | ||
remoteAddress: 'address', | ||
sessionId: 'session', | ||
properties: {'customKey': 'customValue'})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two the exact same? Is that the point? the comments seem to indicate that we're doing different things, but my eyes tell me that these look the same. What am I missing here?
Oh, this is just how we set the initial context, so we're checking that setting the new context (which is exactly the same) doesn't cause an update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a comment explaining this
} else if (key == 'remoteAddress') { | ||
if (context.remoteAddress != newValue) return true; | ||
} else { | ||
if (context.properties[key] != newValue) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this one also handles cases where the key didn't exist before? That is, if context.properties[key]
is null/undefined/whatever it is in dart, then null != "some string" kicks in and we'll run the update, right?
I don't see a unit test for that, but it might be worth adding one (or modifying the current one)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point. I will add a test and I think we're missing field removal in update context
About the changes
Optimize fetch calls when context field(s) don't change.
When you call:
And no field is changed, then we don't make new fetch calls.
Important files
Discussion points